Converts the teeth amount of a gear to the pitch diameter. #72
Conversation
|
Can we add units to all of the methods please? |
| package coppercore.math; | ||
|
|
||
| public class GearConversionFunctions { | ||
| public static double pitchPulley3mm(int teeth) { |
There was a problem hiding this comment.
Per #52 all these functions should return a Distance, which is a unit from wpilib. So that needs to be added to all functions.
Also you'll need to include the following in your build.gradle in the dependencies block
def wpilibVersion = "2025.1.1-beta-1"
implementation "edu.wpi.first.wpiunits:wpiunits-java:$wpilibVersion"
There was a problem hiding this comment.
Comparing floating point numbers with equality is generally considered bad practice. assertEquals should have an option to specify an epsilon, and I recommend setting that to 1e-10.
There was a problem hiding this comment.
To use it just provide a third parameter which it the delta
| @@ -52,6 +52,10 @@ dependencies { | |||
|
|
|||
| // This dependency is used internally, and not exposed to consumers on their own compile classpath. | |||
| implementation libs.guava | |||
|
|
|||
|
|
|||
| def wpilibVersion = "2025.1.1-beta-1" | |||
There was a problem hiding this comment.
This should no longer be the beta version
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class GearConversionTest { | ||
| private static final double delta = 1 * Math.pow(10.0, -10.0); |
There was a problem hiding this comment.
This is fine, but for reference you can do:
private final double kEpsilon = 1e-10;
| Assertions.assertEquals( | ||
| 12 / (Math.PI), GearConversionFunctions.pitchDiameterFrom3mmPulley(4), delta); | ||
| Assertions.assertEquals( | ||
| 150 / (Math.PI), GearConversionFunctions.pitchDiameterFrom3mmPulley(50), delta); | ||
| Assertions.assertEquals( | ||
| 0 / (Math.PI), GearConversionFunctions.pitchDiameterFrom3mmPulley(0), delta); | ||
| Assertions.assertEquals( | ||
| 21 / (Math.PI), GearConversionFunctions.pitchDiameterFrom3mmPulley(-7), delta); |
There was a problem hiding this comment.
Let's make this (and other functions) a little cleaner by using arrays and a loop:
final double[] kInputs = {4.0, 50.0, 0.0, -7.0};
final double[] kExpectedOutputs = {12.0, 150.0, 0.0, 21.0};
for (int i = 0; i < kInputs.length; i++) {
Assertions.assertEquals(kExpectedOutputs[i] / (Math.PI), GearConversionFunctions.pitchDiameterFrom3mmPulley(kInputs[i]), delta);
}
This way the test inputs and expected outputs are super clear, and the test is set up in a simple way that is easy to read
There was a problem hiding this comment.
Is this file actually required? My intuition says no and that it should be deleted. If it is required, it should be upgraded to 2025 though
| import static edu.wpi.first.units.Units.*; | ||
|
|
||
| public class GearConversionFunctions { | ||
| public static double pitchDiameterFrom3mmPulley(int teeth) { |
There was a problem hiding this comment.
These functions likely will fail to compile since we are returning a Distance but saying we will return a double. We should mark this and other functions to return a Distance
No description provided.